-
Notifications
You must be signed in to change notification settings - Fork 49
OneManage: dcnm_fabric_group: module to manage multi-cluster fabric groups #524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
allenrobel
wants to merge
132
commits into
develop
Choose a base branch
from
onemanage-multi-cluster-fabric-group
base: develop
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Description This commit contains an initial implementation for onemanage/fabrics/<fabricName> endpoint. Rather than use the existing inheritance-based endpoint structure, we’ve implemented a composition-based alternative. Several markdown files are included that describe the differences in approach, the benefits of the new approach, a migration guide for existing endpoints, and specifics of the design. These can be used later if/when we decide to migrate the existing endpoints. As a summary, the new approach supports composable query params, including endpoint-specific query params, and Lucene filter query params. ## Markdown files - common/api/QUERY_PARAMS_DESIGN.md - common/api/PROPERTY_STYLE_COMPARISONS.md - common/api/MIGRATION_EXAMPLE.md ## Python files - common/api/base_paths.py - BasePath - centralized base paths - common/api/query_params.py - QueryParams - abstract base class - EndpointQueryParams - endpoint-specific query parameters - LuceneQueryParams - Lucene-Style Query Parameters - common/api/onemanage/endpoints.py - EpOneManageFabricDetails - Fabric Details Endpoint (OneManage)
- common/api/base_paths.py - BasePath: Add classmethods for onemanage/top-down - common/api/onemanage/endpoints - NetworkNamesQueryParams: Query parameters for network deletion endpoints - VrfNamesQueryParams: Query parameters for VRF deletion endpoints - EpOneManageNetworksDelete: Endpoint to bulk-delete of networks - EpOneManageNetworkUpdate: Endpoint to update single Network - EpOneManageVrfsDelete: Endpoint to bulk-delete VRFs - EpOneManageVrfUpdate: Endpoint to update single VRF
Python 3.11 does not support modern enhancements to type hints. Change modern type hints to older-style until we remove support for Python 3.11. ‘str | None’ -> Optional[str] list[Class1 | Class2] -> list[Union[Class1, Class2]]
Update the docstrings with example strings for network_names and vrf_names.
Added installation steps for Pydantic and DeepDiff while removing requests.
Added installation step for Requests library version 2.32.5.
From the sanity section, removed installation steps for Pydantic and Requests since these will be installed in the Docker container now that tests/requirements.txt is present.
Adding tests/requirements.txt so that pydantic and requests will be installed within the docker container on Github when the tests are run.
Let’s see if this works to add pydantic and requests to the sanity test Docker image.
Add pydantic to collection-level requirements.txt
Per Ansible requirements, we need a try/except block around the pydantic import.
Also, remove tests/sanity/requirements.txt since it is not needed.
Per Ansible requirements, add try/except block around pydantic import. Also, update tests/sanity/ignore*.txt to ignore query_params.py import errors.
Create unit tests for common/api/base_paths.py in the following file: tests/unit/module_utils/common/api/test_api_base_paths.py
# Summary Added unit tests for common/api/query_params.py into the file: tests/unit/module_utils/common/api/test_api_query_params.py ## Test Coverage - 48 Tests Total - EndpointQueryParams (8 tests): - to_query_string() with all parameters set - to_query_string() with defaults only - is_empty() with defaults (returns True) - is_empty() with values set (returns False) - to_query_string() with single parameter - to_query_string() with no parameters - model_dump() output validation - _to_camel_case() helper method - LuceneQueryParams (12 tests): - Individual parameters: filter, max, offset, sort, fields - Multiple parameters combined - to_query_string() with various combinations - is_empty() with/without values - Validation: sort direction (asc/desc), max range (1-10000), offset ≥ 0 - Special characters and spaces in filter - Empty/None parameter handling - CompositeQueryParams (10 tests): - add() single parameter object - add() multiple parameter objects - to_query_string() combining endpoint + Lucene params - clear() method - is_empty() validation - Method chaining (add().add()) - Empty composite handling - Order preservation - Validation Error Cases (18 tests): - Invalid sort directions - Invalid max values (0, negative, > 10000) - Invalid offset values (negative) - Type validation for all parameters ## Final Status ✅ All 48 tests passing (1.20s) ✅ Black formatting: passing ✅ isort import ordering: passing ✅ Pylint score: 10.00/10 The test suite follows the same patterns as test_api_base_paths.py with comprehensive docstrings and the does_not_raise() context manager for success cases.
Created comprehensive pytest-based unit tests for api/onemanage/endpoints.py written to tests/unit/module_utils/common/api/onemanage/test_onemanage_endpoints.py. ## Test Coverage - 53 Tests Total ### NetworkNamesQueryParams (6 tests): - to_query_string() with network_names set/unset - is_empty() validation - Pydantic validation for empty strings - Special characters in network names - VrfNamesQueryParams (6 tests): - to_query_string() with vrf_names set/unset - is_empty() validation - Pydantic validation for empty strings - Special characters in VRF names ### EpOneManageFabricCreate (3 tests): - path property - verb property (POST) - class_name attribute ### EpOneManageFabricDetails (6 tests): - path property with fabric_name - ValueError when fabric_name not set - verb property (GET) - class_name attribute - Special characters in fabric_name - Validation for empty fabric_name ### EpOneManageNetworksDelete (7 tests): - path with/without query parameters - ValueError when fabric_name not set - verb property (DELETE) - class_name attribute - query_params initialization - Special characters in names ### EpOneManageNetworkUpdate (9 tests): - path with both parameters set - ValueError when fabric_name not set - ValueError when network_name not set - ValueError when neither parameter set - verb property (PUT) - class_name attribute - Special characters in names - Validation for empty fabric_name/network_name ### EpOneManageVrfsDelete (7 tests): - path with/without query parameters - ValueError when fabric_name not set - verb property (DELETE) - class_name attribute - query_params initialization - Special characters in names ### EpOneManageVrfUpdate (9 tests): - path with both parameters set - ValueError when fabric_name not set - ValueError when vrf_name not set - ValueError when neither parameter set - verb property (PUT) - class_name attribute - Special characters in names - Validation for empty fabric_name/vrf_name Final Status ✅ All 53 tests passing (0.18s) ✅ Black formatting: passing ✅ isort import ordering: passing ✅ Pylint score: 10.00/10 The test suite follows the same comprehensive patterns as test_api_base_paths.py with detailed docstrings and the does_not_raise() context manager for success cases.
It appears pylint version on Github differs from my local version. Appease Github’s pylint by replacing throughout: `_ = endpoint.path` With: endpoint.path # pylint: disable=pointless-statement
Commit Summary # Endpoints Implemented (19 new endpoints) EpOneManageFabricConfigSave - POST fabric config-save EpOneManageFabricConfigPreview - GET fabric config-preview with query params EpOneManageFabricConfigPreviewSwitch - GET switch-level config preview EpOneManageFabricConfigDeploy - POST fabric config-deploy with query params EpOneManageFabricConfigDeploySwitch - POST switch-level config deploy EpOneManageFabricDelete - DELETE fabric EpOneManageFabricGroupUpdate - PUT fabric group membership EpOneManageFabricMembersGet - GET fabric members EpOneManageFabricUpdate - PUT fabric update EpOneManageFabricsGet - GET all fabrics EpOneManageLinkCreate - POST link creation EpOneManageLinksDelete - PUT link deletion EpOneManageLinkGetByUuid - GET link by UUID with query params EpOneManageLinkUpdate - PUT link update EpOneManageLinksGetByFabric - GET links by fabric EpOneManageNetworkCreate - POST network creation EpOneManageNetworksGet - GET all networks EpOneManageVrfsGet - GET all VRFs EpOneManageVrfCreate - POST VRF creation # Query Parameter Classes (3 new) FabricConfigPreviewQueryParams - forceShowRun, showBrief FabricConfigDeployQueryParams - forceShowRun, inclAllMSDSwitches LinkByUuidQueryParams - sourceClusterName, destinationClusterName # Additional Work - Added onemanage_links() helper to BasePath class - Renamed link-related classes for consistency (EpOneManageFabricLink* → EpOneManageLink*) Created 78 new unit tests covering all new endpoints and query parameter classes All 140 tests pass successfully
This commit merely sorts the query parameter and endpont classes by name. No functional changes are made.
Fix pylint trailing-newlines errors. There are no functional changes in this commit.
1. Rename the following class. EpOneManageFabricMembersGet To: EpOneManageFabricGroupMembersGet 2. Update docstrings
The unit tests file was not updated when we renamed EpOneManageFabricMembersGet to EpOneManageFabricGroupMembersGet. This commit fixes this.
We had removed self.log from FabricGroupCommon, but we are still logging things in this class. Add back until we’ve deleted this class.
Privatize several object instances and vars.
- Privatize internal object instances - Remove import and unused instance of FabricGroupTypes - Update class docstring to remove reference to FabricGroupDetails
1. Privatize internal object instances - self.operation_type -> self._operation_type - self.ep_fabric_group_delete -> self._endpoint
The test suite covers all key functionality of the FabricGroups class:
1. Initialization Tests (test_fabric_groups_00000)
- Verifies class attributes are properly initialized
2. Property Tests (tests 00010-00031)
- filter setter/getter
- rest_send setter/getter with error handling
- results setter/getter with error handling
3. Core Functionality Tests (tests 00040-00041)
- refresh() with fabric group data present
- refresh() with no fabric groups on controller
4. Data Access Tests (tests 00050-00062)
- all_data property
- filtered_data property with various scenarios
- Error handling for missing filter
5. Fabric Property Tests (tests 00070-00150)
- asn / bgp_as properties
- deployment_freeze property
- enable_pbr property
- fabric_id property
- fabric_type property
- is_read_only property
- per_vrf_loopback_auto_provision property
- replication_mode property
- template_name property
6. State Management Tests (tests 00160-00171)
- refreshed property before and after refresh
- fabric_group_names property with error handling
7. Private Method Tests (tests 00180-00192)
- _get() method error conditions
- _get_nv_pair() method error conditions
Test Coverage (14 tests total)
The test suite covers all key functionality of the FabricGroupDefault class:
1. Initialization Test (test_fabric_group_default_00000)
- Verifies class attributes are properly initialized
- Validates default values including template_name = "MSD_Fabric"
2. Property Tests (tests 00010-00031)
- fabric_group_name setter/getter
- rest_send setter/getter with validation
- results setter/getter with type checking
- Error handling for invalid property values
3. commit() Method Tests (tests 00040-00050)
- Error handling when fabric_group_name not set
- Error handling when rest_send.params not set
- Successful config building with template retrieval
- Validates complete payload structure
4. config Property Test (test 00060)
- Returns empty dict before commit
- Returns populated config after commit
5. Private Method Tests (tests 00070-00100)
- _skip() method correctly filters parameters
- Skips parameters with "_PREV" suffix
- Skips "DCNM_ID" parameter
- _build_config_top_level() builds correct structure
- _set_parameter_names() extracts parameter names from template
6. Error Handling Test (test 00090)
- Template retrieval failure handling
- Proper exception propagation
Fixture Data
Created a new fixture file responses_TemplateGet.json with:
- Detailed MSD_Fabric template with 12 parameters
- Success response (test_fabric_group_default_00050a)
- Error response for failed template retrieval (test_fabric_group_default_00090a)
Test Patterns Followed
- ✅ Standard pylint directives at the top
- ✅ Proper markdown formatting in docstrings
- ✅ Unique fixture data for each test using the {method_name}a pattern
- ✅ Response generator pattern for mocking REST API calls
- ✅ Proper use of does_not_raise() and pytest.raises()
- ✅ Descriptive variable names (not using _)
- ✅ Comprehensive error condition testing
Quality Checks
All quality checks passed:
- ✅ All 14 tests pass
- ✅ Black formatting (perfect)
- ✅ isort (imports properly sorted)
- ✅ Pylint (10.00/10 rating)
- ✅ Valid JSON in fixture file
- ✅ All 120 fabric_group tests pass together (no conflicts)
Key Features Tested
1. Template Integration: Tests verify the class correctly retrieves and processes the MSD_Fabric template from the controller
2. Parameter Filtering: Tests confirm internal parameters (with "_PREV" suffix or "DCNM_ID") are properly skipped
3. Default Config Building: Tests validate the complete payload structure including nvPairs and top-level attributes
4. Error Handling: Tests ensure proper exception handling for missing dependencies and template retrieval failures
Test Coverage (18 tests total)
The test suite covers all functionality of the FabricGroupTypes class:
1. Initialization Test (test_fabric_group_types_00000)
- Verifies class attributes are properly initialized
- Validates all mapping dictionaries and lists are created
2. Valid Types/Templates Tests (tests 00010-00020)
- valid_fabric_group_types returns sorted list with "MCFG"
- valid_fabric_group_template_names returns sorted list with "MSD_Fabric"
3. fabric_group_type Property Tests (tests 00030-00031)
- Setter/getter functionality
- ValueError when set to invalid type (e.g., "INVALID")
4. template_name Property Tests (tests 00040-00041)
- Returns "MSD_Fabric" for MCFG type
- ValueError when accessed before setting fabric_group_type
5. fabric_type Property Tests (tests 00050-00051)
- Returns "MFD" for MCFG type
- ValueError when accessed before setting fabric_group_type
6. feature_name Property Tests (tests 00060-00061)
- Returns "vxlan" for MCFG type
- ValueError when accessed before setting fabric_group_type
7. mandatory_parameters Property Tests (tests 00070-00071)
- Returns sorted list with "FABRIC_NAME" and "FABRIC_TYPE" for MCFG
- ValueError when accessed before setting fabric_group_type
8. Initialization Method Test (test 00080)
- Verifies _init_fabric_group_types() populates all mappings correctly
- Validates MCFG is properly configured in all maps
9. Integration Test (test 00090)
- Verifies all properties work together correctly
- Tests complete workflow from setting type to accessing all properties
10. Sorting Tests (tests 00100-00110)
- Verifies returned lists are properly sorted
11. Mutability Test (test 00120)
- Verifies fabric_group_type can be changed after initial setting
Test Patterns Followed
- ✅ Standard pylint directives at the top
- ✅ Proper markdown formatting in docstrings
- ✅ Proper use of does_not_raise() and pytest.raises()
- ✅ Descriptive variable names (not using _)
- ✅ Comprehensive property accessor testing
- ✅ Comprehensive error condition testing
Quality Checks
All quality checks passed:
- ✅ All 18 tests pass
- ✅ Black formatting (perfect)
- ✅ isort (imports properly sorted)
- ✅ Pylint (10.00/10 rating)
- ✅ All 138 fabric_group tests pass together (no conflicts)
Key Features Tested
1. Type Validation: Tests verify invalid fabric group types are rejected
2. Property Dependencies: Tests confirm all properties require fabric_group_type to be set first
3. Mapping Consistency: Tests validate all mappings (template, feature, fabric type) are correctly configured
4. Mandatory Parameters: Tests ensure FABRIC_NAME and FABRIC_TYPE are always required
5. Sorting: Tests verify all list properties return sorted results
The tests comprehensively cover the FabricGroupTypes class functionality, following the same patterns and conventions used throughout the codebase.
Properties should check that self.fabric_group_type is set using ‘if not ‘ rather than ‘is None’ since self._fabric_group_type is initialized to an empty string (“”). Fixed.
Since we’ve removed FabricGroupReplaced (fabric_group/replaced.py) we also need to remove it from tests/sanity/ignore-*.txt
To align the behavior with the dcnm_fabric module, we should allow integer or string for integer-like string values. For example, we should accept either of these: DCI_SUBNET_TARGET_MASK: 30 DCI_SUBNET_TARGET_MASK: “30”
Closed
Collaborator
|
Looks like you have a merge conflict to resolve here |
Pull in the good stuff from onemanage-endpoints branch. 1. Import guards from endpoints.py and query_params.py 2. Remove skip tests in tests/sanity/ignore*.txt for endpoints.py and query_params.py 3. Remove custom __init__() methods in endpoints.py 4. Fix Field(None) -> Field(default=None) and similar.
Collaborator
Author
Fixed. |
1. Apply the import guard used in query_params.py to dcnm_fabric_group.py 2. Test commit to see if there’s no need to guard the module_utils imports. 3. Test commit to see if things pass after removing one of the skip directives for plugins/module_utils/fabric_group/common.py May need to revert some of the above if things still fail.
Remove all remaining sanity skip directives for dcnm)fabric_group.
First test to remove module_utils import guard for pydantic (relying on the guard in the main dcnm_fabric_group.py file). If this works, I’ll remove the remaining guards.
Looks like we need the import guard in module_utils as well.
The last attempt caused VS Code to alter imports for other files, which may have caused the errors. Let’s try one more time. Will revert if this doesn’t work…
After testing, the import guard is definitely needed in module_utils files as well :-( Reverting the last commit.
Collaborator
Author
|
All changes from #521 have been applied and import guards are working, so have removed all sanity skip directives for import tests. I do not plan on making many other changes so this is finally ready for review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
An initial implementation of dcnm_fabric_group.
Files Summary
_v2 or _v3 versions of existing files.
For backward compatibility, rather than modify existing files in the module_utils/common and module_utils/fabric directories, we opted to copy them and bump their versions by 1, to either _v2 or _v3.
The _v2 (_v3) files mostly revert the class decorator approach that was used to inject RestSend() and Results() into classes. We’re reverting this approach since mypy does not understand it, and we wanted clean linter results without having to use linter suppression directives all over the place. Instead, we’re duplicating the rest_send and results properties in each class, for now.
Any other changes to existing classes in module_utils/common and module_utils/fabric should be backward compatible, and consist of benign changes, like adding type-hints.
module_utils/fabric_group
All files in this directory are new, and follow the new pattern for RestSend and Results.
modules/dcnm_fabric_group.py
Initial module for fabric group management.
tests/unit/modules/dcnm/dcnm_fabric_group/*
All unit test files in this directory are new.
Status
DONE
Other
TODO
Phase 2 (ND 4.2)